Enhanced routing envchange token support #3646

mdaigle · 2025-09-30T22:55:53Z

Description

Adds enhanced routing envchange support in service of #3641
Changes can be followed commit-by-commit for ease of review.

Issues

#3641

Testing

Adds simulated server tests to verify enhanced routing behavior.

… connection.

…r tests.

…g database value.

paulmedynski · 2025-10-03T13:35:05Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs


                case TdsEnums.FEATUREEXT_ENHANCEDROUTINGSUPPORT:
                    {
                        SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.OnFeatureExtAck|ADV> {0}, Received feature extension acknowledgement for ENHANCEDROUTINGSUPPORT", ObjectID);


This log could be misleading if the data[0] != 1. It implies that enhanced routing is enabled. Perhaps move it below line 3024 and include whether or not the feature is enabled as well.

paulmedynski · 2025-10-03T13:37:28Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

                UserServerName = string.Format(CultureInfo.InvariantCulture, "{0},{1}", routing.ServerName, routing.Port);
            }
+
+            if (routing == null || routing.DatabaseName == null)


Would this statement be easier to understand if it were flipped?

// Always prefer a routing database name from the server, if supplied. if (routing is not null && routing.DatabaseName is not null) { ResolvedDatabaseName = routing.DatabaseName; } else { ResolvedDatabaseName = userOptions.InitialCatalog; }

paulmedynski · 2025-10-03T13:46:12Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/FeatureExtensionEnablementTriState.cs

Maybe FeatureExtensionBehaviour ? It would be funny if you add a 4th member later :)

paulmedynski · 2025-10-03T13:48:39Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs

Shouldn't this depend on how the server intends to respond? If EnableEnhancedRouting is Disabled or DoNotAcknowledge, then support isn't enabled.

Maybe this flag should be called DidClientAskForEnhancedRouting or something more succinct.

paulmedynski · 2025-10-03T13:51:52Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/RoutingTdsServerArguments.cs


+        /// <summary>
+        /// Database name to use at the routed location.
+        /// Should only be set when doing enhanced routing.


"Setting this to a non-empty value will cause the server to include an enhanced routing ENVCHANGE token in the Login Response message. An empty value will include a legacy routing ENVCHANGE token."

paulmedynski · 2025-10-03T13:59:53Z

...rosoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs

+        // Arrange
+        using TdsServer server = new(new()
+        {
+            Log = Console.Out


Our tests should probably be using xUnit's ITestOutputHelper. Just an FYI.

More so, I think we should be avoiding outputting random text during a test anyhow. But I think that's more of an existing issue that we're just not touching today.

paulmedynski · 2025-10-03T14:03:33Z

...rosoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs

+            DataSource = $"localhost,{router.EndPoint.Port}",
+            Encrypt = false,
+            ConnectTimeout = 10000
+        }).ConnectionString;


This arrange stuff is common to both tests. Can you pull it out?

I'll counter that this doesn't meet the rule of 3 for refactoring :)

paulmedynski · 2025-10-03T14:04:21Z

...rosoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs

+        {
+            DataSource = $"localhost,{router.EndPoint.Port}",
+            Encrypt = false,
+            ConnectTimeout = 10000


Do we need to test the case where InitialCatalog is explicitly specified by the app, and ignored during routing?

What about the cases where the server ignores or explicitly denies enhanced routing?

benrr101

Generally speaking it's pretty good. I have a handful of style comments and a couple general questions. But I don't think they're worth holding up the PR if you're not interested (I'm kinda breaking my own rules by not taking a super hard stance on it, tho)

benrr101 · 2025-10-03T19:14:58Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

                    break;

+                case TdsEnums.ENV_ENHANCEDROUTING:
+                    SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.OnEnvChange|ADV> {0}, Received enhanced routing info", ObjectID);


I think the tracing format we're trying to use going forward is the xyz | abc | whatever format. But if the rest of the strings in this file are using the <abc...> asdfasdfads format, then I won't complain.

benrr101 · 2025-10-03T19:16:14Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

+
+            if (routing == null || routing.DatabaseName == null)
+            {
+                ResolvedDatabaseName = userOptions.InitialCatalog;


I can has ternary ? 👉👈🥺

benrr101 · 2025-10-03T19:19:40Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

-                        if (result != TdsOperationStatus.Done)
-                        {
-                            return result;
+                            ushort newLength;


So the whole deal with this big block o changes was you added scope {} to this case? ... If we can avoid doing that that'd be better. But, if you reallly reallllly want to use a new variable in two scopes, I'll let it slide

benrr101 · 2025-10-03T19:21:14Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

+        /// <returns>Returns a <see cref="TdsOperationStatus"/> to indicate the status of the operation.</returns>
+        private TdsOperationStatus TryProcessEnhancedRoutingToken(TdsParserStateObject stateObj, SqlEnvChange env)
+        {
+            ushort newLength;


Can we inline the declarations here?

benrr101 · 2025-10-03T19:23:55Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

        }

+        /// <summary>
+        /// Reads an enhanced routing token from the stream and populates the provided <see cref="SqlEnvChange"/> object.


Not required, but I think it'd be really nice to have a layout of the token structure in a comment either a remarks or comment in the body, maybe?

benrr101 · 2025-10-03T19:37:36Z

...rosoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs

+            DataSource = $"localhost,{router.EndPoint.Port}",
+            Encrypt = false,
+            ConnectTimeout = 10000
+        }).ConnectionString;


I'll counter that this doesn't meet the rule of 3 for refactoring :)

benrr101 · 2025-10-03T19:38:43Z

...soft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs

+
+        // Act
+        sqlConnection.Open();
+


nit: extra line here and in below instances

benrr101 · 2025-10-03T19:39:14Z

...soft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs

+    }
+}
+
+public class SimulatedServerFixture : IDisposable


Please put top level classes in their own files

benrr101 · 2025-10-03T19:41:57Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/FeatureExtensionEnablementTriState.cs

+
+namespace Microsoft.SqlServer.TDS.Servers
+{
+    public enum FeatureExtensionEnablementTriState


Can we do this as effectively with a bool? ?

benrr101 · 2025-10-03T19:42:10Z

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs

+                // Create ack data (1 byte: IsEnabled)
+                byte[] data = new byte[1];
+
+                if (EnableEnhancedRouting == FeatureExtensionEnablementTriState.Enabled)


Can has ternary again?

In fact, can just do

byte[] data = EnableEnhancedRouting is FeatureExtensionEnablementTristate.Enabled ? [1] : [0];

(and if we switch from tristate to bool? we could condense even more to)

byte[] data = IsEnhancedRoutingEnabled ? [1] : [0]

mdaigle added 20 commits September 29, 2025 13:04

Add feature extension, request by default, process ack.

4c0acd3

Add support to netfx. Add simulated server tests.

2136370

review changes

40af1cf

Add enhanced routing to TdsEnums.

924218a

Remove unused envchange enum.

fabad0a

Add database name to RoutingInfo dto.

702ecf1

Add new user facing exception message for invalid enhanced routing info.

91bade2

Add enhanced routing token processing from stream.

eb8d271

Add logic to validate and store enhanced routing info on the internal…

5aff345

… connection.

Convert some fields to auto properties to reduce clutter.

b850421

Clean up argument casting in RoutingTdsServer.

2fc8175

Add database name to routing server arguments.

7dcafb8

Add enhanced routing envchange token value for use by simulated serve…

1d9b7dd

…r tests.

Add enhanced routing envchange type to enum

2d422fb

Return enhanced routing token when server is configured with a routin…

2456f28

…g database value.

Enable default compile items for TDS proj.

0b6289d

Clean up envchange token initialization.

8cd2270

Fix ResolvedDatabaseName update.

b1aee5e

Fix enhanced routing inflation and deflation for simulated servers.

05ca818

Add simulated server tests.

3486a2f

mdaigle requested a review from a team as a code owner September 30, 2025 22:55

Base automatically changed from dev/mdaigle/enhanced-routing-feature-extension to feat/enhanced-routing October 2, 2025 16:29

paulmedynski requested changes Oct 3, 2025

View reviewed changes

paulmedynski self-assigned this Oct 3, 2025

benrr101 approved these changes Oct 3, 2025

View reviewed changes

Enhanced routing envchange token support #3646

Are you sure you want to change the base?

Enhanced routing envchange token support #3646

Conversation

mdaigle commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benrr101 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdaigle commented Sep 30, 2025 •

edited

Loading

benrr101 left a comment •

edited

Loading